-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Hover to TabItem #28357
Add Hover to TabItem #28357
Conversation
…x/26648-3-tab-hover
…x/26648-3-tab-hover
…x/26648-3-tab-hover
…dan-m/App into wildan/fix/26648-3-tab-hover
…x/26648-3-tab-hover
…dan-m/App into wildan/fix/26648-3-tab-hover
…x/26648-3-tab-hover
…dan-m/App into wildan/fix/26648-3-tab-hover
…x/26648-3-tab-hover
…dan-m/App into wildan/fix/26648-3-tab-hover
@@ -82,7 +82,7 @@ const getBackgroundColor = (position, routesLength, tabIndex) => { | |||
|
|||
function TabSelector({state, navigation, onTabPress, position}) { | |||
const {translate} = useLocalize(); | |||
|
|||
const hoverBackgroundColor = themeColors.highlightBG; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we defining this here and passing it as a prop rather than just passing the value inside TabSelectorItem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we defining this here and passing it as a prop rather than just passing the value inside TabSelectorItem?
I'm trying to make it consistent with how backgroundColor
passed, also it can be a tiny feature in case we need to customize the tabitem hover color.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if that is not required we can directly use styles.highlightBG
in the tabitem inner animated.view
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ollyws , sorry I misunderstood your question, yes, we can simplify it without declaring a variable. it just updated.
@wildan-m Just wondering, did you do much testing with your original solution? Were there any performance drawbacks? Because it does seem a bit tidier to just have the Animated.View with the background and non-Animated pressable. |
@Ollyws Yes I did. How do I test it? I didn't involve hoverable changes in this test, just made changes in the AnimatedPressableWithFeedback. This is three attempts profiling result to move the tab from
|
@wildan-m Ahh thanks, nice. Does this give a performance increase over using a hoverable? |
@Ollyws that's weird, isn't it? Even when we pass the hovered param to tabicon/label, the issue is occurred (Alternative 2) For me, the only working solution that resolves all of these issues is this one: (don't forget to import StyleSheet from RN)
styles
using Hoverable mixed with animated components might skip important render. are we okay with using the above change instead? |
@wildan-m Yeah go ahead, thanks. |
@Ollyws updated |
@wildan-m Great, that seems to fix the issue. diff --git a/src/components/TabSelector/TabSelectorItem.js b/src/components/TabSelector/TabSelectorItem.js
index 168a783ad6..6611b8acf9 100644
--- a/src/components/TabSelector/TabSelectorItem.js
+++ b/src/components/TabSelector/TabSelectorItem.js
@@ -1,4 +1,4 @@
-import {Animated, View, StyleSheet} from 'react-native';
+import {Animated, StyleSheet} from 'react-native';
import React from 'react';
import PropTypes from 'prop-types';
import styles from '../../styles/styles';
@@ -42,18 +42,16 @@ const defaultProps = {
isFocused: false,
};
-const AnimatedPressableWithFeedback = Animated.createAnimatedComponent(PressableWithFeedback);
-
function TabSelectorItem({icon, title, onPress, backgroundColor, activeOpacity, inactiveOpacity, isFocused}) {
return (
- <AnimatedPressableWithFeedback
+ <PressableWithFeedback
accessibilityLabel={title}
- style={[styles.tabSelectorButton, {backgroundColor}]}
+ style={[styles.tabSelectorButton]}
wrapperStyle={[styles.flex1]}
onPress={onPress}
>
{({hovered}) => (
- <View style={[styles.tabSelectorButton, StyleSheet.absoluteFill, styles.tabBackground(hovered, isFocused, undefined)]}>
+ <Animated.View style={[styles.tabSelectorButton, StyleSheet.absoluteFill, styles.tabBackground(hovered, isFocused, backgroundColor)]}>
<TabIcon
icon={icon}
activeOpacity={styles.tabOpacity(hovered, isFocused, activeOpacity, inactiveOpacity)}
@@ -64,9 +62,9 @@ function TabSelectorItem({icon, title, onPress, backgroundColor, activeOpacity,
activeOpacity={styles.tabOpacity(hovered, isFocused, activeOpacity, inactiveOpacity)}
inactiveOpacity={styles.tabOpacity(hovered, isFocused, inactiveOpacity, activeOpacity)}
/>
- </View>
+ </Animated.View>
)}
- </AnimatedPressableWithFeedback>
+ </PressableWithFeedback>
);
}
diff --git a/src/styles/styles.js b/src/styles/styles.js
index 81b3b139dc..418d76b67f 100644
--- a/src/styles/styles.js
+++ b/src/styles/styles.js
@@ -3450,8 +3450,8 @@ const styles = (theme) => ({
color: isSelected ? theme.textLight : theme.textSupporting,
}),
- tabBackground: (hovered, isFocused) => ({
- backgroundColor: hovered && !isFocused ? theme.highlightBG : undefined,
+ tabBackground: (hovered, isFocused, background) => ({
+ backgroundColor: hovered && !isFocused ? theme.highlightBG : background,
}),
tabOpacity: (hovered, isFocused, activeOpacityValue, inactiveOpacityValue) => (hovered && !isFocused ? inactiveOpacityValue : activeOpacityValue),
Diff |
@wildan-m Yeah that's just a miniscule bump to render time, I don't think it's reflective of the animation performance and tidy code is worth it. |
…x/26648-3-tab-hover
@Ollyws sure, change has been applied |
Reviewer Checklist
Screenshots/VideosWeb01_MacOS_Chrome.mp4Mobile Web - Chrome02_Android_Chrome.mp4Mobile Web - Safari03_iOS_Safari.mp4Desktop04_MacOS_Desktop.mp4iOS05_iOS_Native.mp4Android06_Android_Native.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Thanks for all the changes!
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/MariaHCD in version: 1.3.80-0 🚀
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.80-3 🚀
|
🚀 Deployed to staging by https://github.com/MariaHCD in version: 1.3.81-0 🚀
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.83-11 🚀
|
2 similar comments
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.83-11 🚀
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.83-11 🚀
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.83-11 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.3.84-10 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.3.84-10 🚀
|
FYI, This PR caused #47381 you can a find RCA in this proposal: #47381 (comment) |
@Ollyws, @MariaHCD, @garrettmknight
Details
Add tab item styling when hovered
Fixed Issues
$ #26648
PROPOSAL: #26648 (comment)
Tests
Web / Desktop
Scan
tab as the initial tabManual
tabManual
tab background color is equal to themeColors.highlightBG / darkHighlightBackground / #07271FManual
tabScan
tabScan
tab background color is equal to themeColors.highlightBG / darkHighlightBackground / #07271FNative / Mobile Web
Scan
tab as the initial tabManual
tabOffline tests
Web / Desktop
Scan
tab as the initial tabManual
tabManual
tab background color is equal to themeColors.highlightBG / darkHighlightBackground / #07271FManual
tabScan
tabScan
tab background color is equal to themeColors.highlightBG / darkHighlightBackground / #07271FNative / Mobile Web
Scan
tab as the initial tabManual
tabQA Steps
Web / Desktop
Scan
tab as the initial tabManual
tabManual
tab background color is equal to themeColors.highlightBG / darkHighlightBackground / #07271FManual
tabScan
tabScan
tab background color is equal to themeColors.highlightBG / darkHighlightBackground / #07271FNative / Mobile Web
Scan
tab as the initial tabManual
tabPR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Recording.79.mp4
Mobile Web - Chrome
Recording.81.mp4
Mobile Web - Safari
Recording.83.mp4
Desktop
Recording.84.mp4
iOS
Recording.82.mp4
Android
Recording.80.mp4